Add helper output for connecting after running add-to-ssh-agent #32

imrehg · 2020-03-19T15:36:59Z

The function definition header has help how to connect to the server whose key we've added to the ssh-agent, on the other hand the tabular output is both different order and would still require knowledge of the ssh parameters.

This change fills in the command with the server variables and adds some extra flags that help to be less noisy later: not adding the server's key to the known hosts file, and not checking the server's key.

Hostname          Port  Username
123.123.123.123   12345  faculty

Connect to the server by:
ssh [email protected] -p 12345 -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no

It should work both on Linux/Mac and Windows as the relevant null device is substituted in the command example.

Signed-off-by: Gergely Imreh [email protected]

The function definition header has help how to connect to the server whose key we've added to the ssh-agent, on the other hand the tabular output is both different order and would still require knowledge of the ssh parameters. This change fills in the command with the server variables and adds some extra flags that help to be less noisy later: not adding the server's key to the known hosts file, and not checking the server's key. ``` Hostname Port Username 123.123.123.123 12345 faculty Connect to the server by: ssh [email protected] -p 12345 -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no ``` It should work both on Linux/Mac and Windows as the relevant null device is substituted in the command example. Signed-off-by: Gergely Imreh <[email protected]>

Signed-off-by: Gergely Imreh <[email protected]>

Since older Pythons don't do f-strings, duh. Signed-off-by: Gergely Imreh <[email protected]>

Otherwise can run into issues that black formats lines one way, but flake8 doesn't accept the formatting choices. Signed-off-by: Gergely Imreh <[email protected]>

…on 2 This reverts commit fe1bfcb.

Signed-off-by: Gergely Imreh <[email protected]>

srstevenson · 2020-03-19T16:26:27Z

What's the intended use case for this? I don't see an advantage of executing the printed SSH command over using faculty shell <project> <container>.

imrehg · 2020-03-19T16:40:30Z

One of the recent PRs added recently added this function of adding the remote server's key to the ssh-agent. The output of that function gives that host/port/username table, and the docstring of the function has hint how to use it. Don't think anyone will go there to check, if the function outputs those server infos, then should be able to use it easily. I agree with using "faculty shell". Here I am trying to improve on the usability of "faculty server add-to-ssh-agent", since someone already needed it, then make it easier?

…

On Thu, Mar 19, 2020, 16:26 Scott Stevenson ***@***.***> wrote: What's the intended use case for this? I don't see an advantage of executing the printed SSH command over using faculty shell <project> <container>. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#32 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAJPT347RCC7AHX2ZUDNXLRIJBUFANCNFSM4LPLZNRA> .

srstevenson · 2020-03-21T14:51:42Z

The add-to-ssh-agent command was primarily added to support using a remote interpreter with VS Code and PyCharm. PyCharm requires each of the connection details to be input separately:

VS Code requires something like the connection string this PR adds, but doesn't need the ssh prefix or the trailing options:

faculty_cli/cli.py

tox.ini

The awkward string formatting is necessary as `black` doesn't break long strings but `flake8` despises them. Signed-off-by: Gergely Imreh <[email protected]>

imrehg · 2020-03-21T15:35:49Z

@srstevenson I am okay to just drop this whole thing, if it's not needed. Your comment make sense, though then I'm not reconciled with the what the docstring in the code says, which also misleads about the use case (as it projects as ssh is the main use for why that function is added in the first place)

faculty-cli/faculty_cli/cli.py

Lines 721 to 722 in 96b83ab

    
           After running this command, SSH into the server using 
        
           `ssh <username>@<hostname> -p <port>`.

Based on your comment, I guess that docsting needs an update as well, or removing that part entirely, and documenting for the CLI the use case with the IDEs you mentioned + recommending faculty shell for all other uses just as you said. Start to feel that is a much better way then what this PR is trying to achieve (which I start to file under misguided attempts to help).

srstevenson · 2020-03-21T15:42:08Z

Pinging @liamcoatman and @brynmathias for thoughts, as they're more familiar with the use case for this than me.

Signed-off-by: Gergely Imreh <[email protected]>

imrehg · 2020-03-21T15:51:22Z

Btw, great, that ignoring the long line error (E501) just makes a bunch of warnings (W503) an error it seems, and that's coming from black's formatting :P

Signed-off-by: Gergely Imreh <[email protected]>

liamcoatman · 2020-03-21T18:54:46Z

I agree the example could confuse the user about the intended use case (and the overlap in functionality with faculty shell...). For this reason I would just remove the lines:

After running this command, SSH into the server using
    `ssh <username>@<hostname> -p <port>`.

I don't think any other commands have overly verbose documentation, so we should probably follow that example.

acroz

I agree with other commenters in that I don't understand the use case for this addition. Adding this output implies that running this command then manually SSHing to the server is a recommended flow, however I think we want to encourage people to continue to use faculty shell.

I think it would be useful for my understanding if you could propose a use case, even if hypothetical.

Aside from this I've left a few minor comments on the implementation.

acroz · 2020-03-21T21:28:06Z

tox.ini

+[flake8]
+# Ignore long line errors, and rely on the formatting done properly by black
+# Also ignore "Line break occurred before a binary operator" also coming from black
+ignore = E501,W503


Can we instead put this in setup.cfg, as in https://github.com/facultyai/faculty/blob/master/setup.cfg ?

It should then get picked up by linter plugins in peoples' editors.

That's good info, thanks! Just getting to learn the number of different configs that one can use to set these tooling (just like pyproject.toml mentioned earlier). Will try that out too, and it makes complete sense putting it where it has the best effect.

acroz · 2020-03-21T21:30:23Z

faculty_cli/cli.py

@@ -1158,7 +1165,7 @@ def put(project, local, remote, server):
                "-P",
                str(details.port),
                os.path.expanduser(local),
-                u"{}@{}:{}".format(
+                "{}@{}:{}".format(


We've not yet made the decision to drop Python 2 support. Can we leave these unicode literals until then?

Looking back through the changes, I believe this is done by a newer version of black (than defined in tox.ini) cleaning that up, and when I realised that I run locally a different version and downgraded, I didn't go back to undo the changes that the original (version 19?) done. Totally makes sense, and will watch out for this later.

acroz · 2020-03-21T21:35:08Z

faculty_cli/cli.py

+    click.echo(
+        "\n"
+        + "Login to the server with:\n"
+        + "ssh {}@{} -p {} -o UserKnownHostsFile={} -o StrictHostKeyChecking=no".format(
+            details.username, details.hostname, details.port, os.devnull
+        )
+    )


Might this be a little tidier as:

Suggested change

click.echo(

"\n"

+ "Login to the server with:\n"

+ "ssh {}@{} -p {} -o UserKnownHostsFile={} -o StrictHostKeyChecking=no".format(

details.username, details.hostname, details.port, os.devnull

)

)

click.echo("")

click.echo("Login to the server with:")

click.echo(

"ssh {}@{} -p {} -o UserKnownHostsFile={} -o StrictHostKeyChecking=no".format(

details.username, details.hostname, details.port, os.devnull

)

)

We already compose the output like this in https://github.com/facultyai/faculty-cli/blob/master/faculty_cli/cli.py#L851

Ah, thanks for the example! I was wondering of a setup like this, but haven't looked deep enough. Makes complete sense!

imrehg · 2020-03-23T14:36:46Z

Hey, thanks for the feedback everyone. I guess I dived in this without much critical thinking, rather "improvements for improvements' sake". Based on the different current uses. mentioned, indeed there's no need for the core of this PR at all. I'm just going to close this.

There might be a handful of housekeeping improvements (not related to the ssh part, but popped up while developing this PR, such as the flake8-black potential tug-o-war) as discussed in the comments, but those don't seem pressing or completely clear cut beneficial just yet.

Cheers for all the teaching, much appreciated!

acroz · 2020-03-24T10:58:17Z

Sorry this did not make it through @imrehg - we appreciate your taking the initiative to further improve the tool!

imrehg added 7 commits March 19, 2020 15:35

Formatting changes, not connected to the main code, required by black

b651327

Signed-off-by: Gergely Imreh <[email protected]>

Redo string output with format

d5da1d8

Since older Pythons don't do f-strings, duh. Signed-off-by: Gergely Imreh <[email protected]>

Update flake8 check to follow black's formatting

fe1bfcb

Otherwise can run into issues that black formats lines one way, but flake8 doesn't accept the formatting choices. Signed-off-by: Gergely Imreh <[email protected]>

Revert "Update flake8 check to follow black's formatting" due to Pyth…

53d53d2

…on 2 This reverts commit fe1bfcb.

Simplify output formatting and linter setup.

a0209d7

Signed-off-by: Gergely Imreh <[email protected]>

More formatting, as newer black does things differently.

2e8eec9

Signed-off-by: Gergely Imreh <[email protected]>

srstevenson suggested changes Mar 21, 2020

View reviewed changes

faculty_cli/cli.py Outdated Show resolved Hide resolved

tox.ini Outdated Show resolved Hide resolved

Remove ineffective flake8 setting, update grammar, reformat string

90b46ff

The awkward string formatting is necessary as `black` doesn't break long strings but `flake8` despises them. Signed-off-by: Gergely Imreh <[email protected]>

Instruct flake8 to ignore long line errors, and string formatting

1d6d9a9

Signed-off-by: Gergely Imreh <[email protected]>

Add another ignore to flake8, for usability and relying on black

ef3e309

Signed-off-by: Gergely Imreh <[email protected]>

acroz reviewed Mar 21, 2020

View reviewed changes

imrehg closed this Mar 23, 2020

imrehg deleted the ssh-connect branch March 23, 2020 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add helper output for connecting after running add-to-ssh-agent #32

Add helper output for connecting after running add-to-ssh-agent #32

imrehg commented Mar 19, 2020

srstevenson commented Mar 19, 2020

imrehg commented Mar 19, 2020 via email

srstevenson commented Mar 21, 2020

imrehg commented Mar 21, 2020

srstevenson commented Mar 21, 2020

imrehg commented Mar 21, 2020

liamcoatman commented Mar 21, 2020

acroz left a comment

acroz Mar 21, 2020

imrehg Mar 23, 2020

acroz Mar 21, 2020

imrehg Mar 23, 2020

acroz Mar 21, 2020

imrehg Mar 23, 2020

imrehg commented Mar 23, 2020

acroz commented Mar 24, 2020

Add helper output for connecting after running add-to-ssh-agent #32

Add helper output for connecting after running add-to-ssh-agent #32

Conversation

imrehg commented Mar 19, 2020

srstevenson commented Mar 19, 2020

imrehg commented Mar 19, 2020 via email

srstevenson commented Mar 21, 2020

imrehg commented Mar 21, 2020

srstevenson commented Mar 21, 2020

imrehg commented Mar 21, 2020

liamcoatman commented Mar 21, 2020

acroz left a comment

Choose a reason for hiding this comment

acroz Mar 21, 2020

Choose a reason for hiding this comment

imrehg Mar 23, 2020

Choose a reason for hiding this comment

acroz Mar 21, 2020

Choose a reason for hiding this comment

imrehg Mar 23, 2020

Choose a reason for hiding this comment

acroz Mar 21, 2020

Choose a reason for hiding this comment

imrehg Mar 23, 2020

Choose a reason for hiding this comment

imrehg commented Mar 23, 2020

acroz commented Mar 24, 2020